Skip to content

Conversation

@fivetran-jamie
Copy link
Contributor

@fivetran-jamie fivetran-jamie commented Jun 30, 2025

PR Overview

Package version introduced in this PR:
v0.9.1

This PR addresses the following Issue/Feature(s):

Summary of changes:

Creates new variables to disable the issue_assignee, issue_label, label, and requested_reviewer_history models if needed. Appropriately creates null fields or totally excludes fields/joins if the tables are missing.

Submission Checklist

  • Alignment meeting with the reviewer (if needed)
    • Timeline and validation requirements discussed
  • Provide validation details:
    • Validation Steps: Check for unintentional effects (e.g., add/run consistency & integrity tests)
      Consistency tests added for all models, passing
image
  • Testing Instructions: Confirm the change addresses the issue(s)

I'll share more detailed screenshots in Height, but without configuring the variables (default value = True) dbt run produces 43 models
image

With the variables set to False, dbt run produces 32 models
image

  • 8 of these are from the staging layer, as highlighted in the source PR

  • The remaining 3 are int_github__issue_assignees, int_github__issue_labels, and int_github__issue_label_joined

    • Focus Areas: Complex logic or queries that need extra attention

Changelog

  • Draft changelog for PR
  • Final changelog for release review

@fivetran-jamie fivetran-jamie changed the title working Add disabling variables for issue_assignee, issue_label, label, and requested_reviewer_history Jun 30, 2025
@fivetran-jamie fivetran-jamie self-assigned this Jun 30, 2025
@fivetran-jamie fivetran-jamie marked this pull request as ready for review June 30, 2025 19:41
Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments and requests before approval

creator.name as creator_name,
creator.company as creator_company,

{%- if var('github__using_requested_reviewer_history', True) -%}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's a bit out of scope of this task, but can you please add the fully qualified name of these fields. Some fields have the fully qualified name (eg. issue_assignees.assignees) whereas others do not (hours_request_review_to_first_review). It makes it difficult to trace back where these fields are coming from.

No need to make this update across all the files, but for the ones being updated please make this update to make it easier to maintain in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated!

Comment on lines 97 to 99
hours_request_review_to_first_review,
hours_request_review_to_first_action,
hours_request_review_to_merge,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For fields like this that are no longer going to persist to the end model if the variables are disabled, we should updated the documentation accordingly to highlight that these fields will only be present with certain variables enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated and regenerated docs

Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

packages.yml Outdated
Comment on lines 2 to 5
# - package: fivetran/github_source
# version: [">=0.9.0", "<0.10.0"]
- git: https://github.com/fivetran/dbt_github_source.git
revision: feature/add-more-table-vars
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder to swap before merge

dbt deps
dbt seed --target "$db" --full-refresh
dbt source freshness --target "$db" || echo "...Only verifying freshness runs…"
dbt run --vars '{github__using_repo_team: false, github__using_issue_assignee: false, github__using_issue_label: false, github__using_requested_reviewer_history: false}' --target "$db" --full-refresh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dbt run --vars '{github__using_repo_team: false, github__using_issue_assignee: false, github__using_issue_label: false, github__using_requested_reviewer_history: false}' --target "$db" --full-refresh
dbt run --vars '{github__using_repo_team: false, github__using_issue_assignee: false, github__using_issue_label: false, github__using_requested_reviewer_history: false, github__using_label: false}' --target "$db" --full-refresh

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch -- updated myself directly

dbt seed --target "$db" --full-refresh
dbt source freshness --target "$db" || echo "...Only verifying freshness runs…"
dbt run --vars '{github__using_repo_team: false, github__using_issue_assignee: false, github__using_issue_label: false, github__using_requested_reviewer_history: false}' --target "$db" --full-refresh
dbt test --vars '{github__using_repo_team: false, github__using_issue_assignee: false, github__using_issue_label: false, github__using_requested_reviewer_history: false}' --target "$db"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dbt test --vars '{github__using_repo_team: false, github__using_issue_assignee: false, github__using_issue_label: false, github__using_requested_reviewer_history: false}' --target "$db"
dbt test --vars '{github__using_repo_team: false, github__using_issue_assignee: false, github__using_issue_label: false, github__using_requested_reviewer_history: false, github__using_label: false}' --target "$db"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated myself directly

Comment on lines +89 to +92
github__using_issue_assignee: false # by default this is assumed to be true
github__using_issue_label: false # by default this is assumed to be true
github__using_label: false # by default this is assumed to be true
github__using_requested_reviewer_history: false # by default this is assumed to be true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we update line 84 with the similar tables not being synced?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep updated!

Copy link
Contributor

@fivetran-avinash fivetran-avinash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fivetran-jamie Few comments before approval

Copy link
Contributor

@fivetran-avinash fivetran-avinash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fivetran-jamie Approved!

@fivetran-jamie fivetran-jamie merged commit f68f209 into main Jul 2, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants